fixed issue #1080, re-enable failing udp tests on windows #1167

dtacalau · 2019-11-25T14:45:17Z

Fixes issues #1080 & #1131.

Couple of fixes two re-enable two udp tests which were failing on windows.
Added two new tests to check ET for UDP sockets, tests rely on a WouldBlock recv(_from) to reregister the socket and reset the interests.
Added a note in the read/peek(_from) docs that on Windows reading with a too small a buffer will return an error.

No need to emulate edge-triggers for UDP sockets by doing reregister on partial reads/write.

dtacalau · 2019-11-25T16:34:45Z

@Thomasdezeeuw please review, thanks!

tests/util/mod.rs

src/sys/windows/udp.rs

Thomasdezeeuw · 2019-11-25T17:01:59Z

src/sys/windows/udp.rs

    }

    pub fn recv_from(&self, buf: &mut [u8]) -> io::Result<(usize, SocketAddr)> {
-        try_io!(self, recv_from, buf)
+        try_io!(self.recv_from(buf), |t: &(usize, SocketAddr)| match t {
+            (bytes_read, _) => *bytes_read <= buf.len(),


UDP doesn't work the same way TCP does. Calling recv(_from) will receive a single packet. Because of thisbuf is usually 1 << 16 bytes (~65k, the maximum size of a single packet) and most read packets will be smaller then that. Which means that it will always re-register, which emulating level triggers.

In this case we need to always reregister for recv(_from) on successfull op.

reverted this change, not needed.

Thomasdezeeuw · 2019-11-27T11:21:29Z

src/sys/windows/udp.rs

    }

    pub fn peek(&self, buf: &mut [u8]) -> io::Result<usize> {
        try_io!(self, peek, buf)
    }

    pub fn connect(&self, addr: SocketAddr) -> io::Result<()> {
-        self.inner.connect(addr)
+        let result = self.inner.connect(addr);


Are you sure this is required?

Don't think its required, see comment in udp_socket_edge_triggered_behavior test.

you're right, not required.

Thomasdezeeuw · 2019-11-27T11:26:33Z

tests/udp_socket.rs

+        expect_read!(socket1.recv(&mut buf[..max]), &DATA2[..max]);
+    }
+    if cfg!(target_os = "windows") {
+        // On Windows, read all data, reading part of data returns error WSAEMSGSIZE (10040)


Let's not split up this test in Windows/others. Lets just read the entire packet on all platform.

Also lets add a note in the read/peek(_from) docs that on Windows reading with a too small a buffer will return an error. Encouraging people to use 1 << 16 sized buffers (the maximum size for a UDP packet).

ok, will read all

I've added note for all read/peek(_from)

tests/udp_socket.rs

Thomasdezeeuw · 2019-11-27T11:27:00Z

tests/udp_socket.rs

@@ -387,7 +391,15 @@ fn unconnected_udp_socket_connected_methods() {
    );

    // Socket is unconnected, but we're using an connected method.
-    assert_error(socket1.send(DATA1), "address required");
+    if cfg!(not(target_os = "windows")) {


Same here as above.

Not sure if possible, there are different error messages on Windows vs other platforms. How would you have one assert checking two different expected messages?

I don't think mio makes any guarantees as to error messages. It doesn't matter terribly to me if the test only check that an error is received or it adds cfg flags to test messages per platform.

You can just change this to assert_error(socket1.send(DATA1), "address") and add a comment saying it must say something about missing an address. Then we don't have this complex cfg! stuff.

I'd rather not change this, unless you have a strong reason against it. Having cfg! in tests doesn't feel complex at all, actually it's weird we don't have more cfg! in them. Having just an assert_error(socket1.send(DATA1), "address") doesn't say much about the actual error you are expecting there. "address required" is enough to get an idea, "no address was supplied. (os error 10057)" is even better. IMO tests should be as explicit as possible.

tests/udp_socket.rs

tests/util/mod.rs

Thomasdezeeuw · 2019-11-27T11:31:16Z

src/sys/windows/udp.rs

    }

    pub fn peek(&self, buf: &mut [u8]) -> io::Result<usize> {
        try_io!(self, peek, buf)
    }

    pub fn connect(&self, addr: SocketAddr) -> io::Result<()> {
-        self.inner.connect(addr)
+        let result = self.inner.connect(addr);


Don't think its required, see comment in udp_socket_edge_triggered_behavior test.

dtacalau · 2019-11-27T12:49:01Z

@Thomasdezeeuw I found some interesting things related to ET while playing with tests. This PR depends on the conclusion from here #1131 (comment)

Signed-off-by: Daniel Tacalau <[email protected]>

dtacalau · 2019-11-27T21:03:22Z

@carllerche please review, thanks!

dtacalau · 2019-11-27T21:21:20Z

#1174 opened to investigate *nix behavior.

carllerche

LGTM.

…ndows. One tests needed to have interests reregistered by calling a blocking recv. Another tests needed a Windows specific assert error message. Signed-off-by: Daniel Tacalau <[email protected]>

… is larger than the buffer specified, the buffer is filled with the first part of the data, and peek returns the error WSAEMSGSIZE(10040). The excess data is lost. Make sure to always use a sufficiently large buffer to hold the maximum UDP packet size, which can be up to 64 kilobytes in size. Signed-off-by: Daniel Tacalau <[email protected]>

…se tests rely on having a blocking recv(_from) reregistering the socket and reseting the interests. Signed-off-by: Daniel Tacalau <[email protected]>

Thomasdezeeuw

Some small nits, but looks good overall.

Thomasdezeeuw · 2019-11-28T14:06:57Z

src/net/udp.rs

+    /// is filled with the first part of the data, and recv_from returns the error
+    /// WSAEMSGSIZE(10040). The excess data is lost.
+    /// Make sure to always use a sufficiently large buffer to hold the
+    /// maximum UDP packet size, which can be up to 64 kilobytes in size.


Its actually 65536 bytes, lets just use 1 << 16.

Same for the other notes.

64 kilobytes is 65536 bytes. I will replace with 65536 bytes, which is the most explicit, I dont find 1 << 16 better.

Thomasdezeeuw · 2019-11-28T14:10:15Z

tests/udp_socket.rs

@@ -387,7 +391,15 @@ fn unconnected_udp_socket_connected_methods() {
    );

    // Socket is unconnected, but we're using an connected method.
-    assert_error(socket1.send(DATA1), "address required");
+    if cfg!(not(target_os = "windows")) {


You can just change this to assert_error(socket1.send(DATA1), "address") and add a comment saying it must say something about missing an address. Then we don't have this complex cfg! stuff.

Thomasdezeeuw · 2019-11-28T14:12:17Z

tests/udp_socket.rs

+        socket1.connect(address2).unwrap();
+
+        let mut buf = [0; 20];
+        checked_write!(socket1.send(DATA1));


Writing without receiving an writable event first.

ok, will expect writable before

Thomasdezeeuw · 2019-11-28T14:13:06Z

tests/udp_socket.rs

+        vec![ExpectEvent::new(ID2, Interests::WRITABLE)],
+    );
+
+    {


Don't think this extra block is needed.

ok, removed

Signed-off-by: Daniel Tacalau <[email protected]>

… a writable before actual write Signed-off-by: Daniel Tacalau <[email protected]>

dtacalau · 2019-11-28T15:47:32Z

Some small nits, but looks good overall.

@Thomasdezeeuw fixed some of them.

…kio-rs#1167) * fixed assert arguments order Signed-off-by: Daniel Tacalau <[email protected]> * fix issue tokio-rs#1080, re-enabled and fixed UDP tests failing on Windows. One tests needed to have interests reregistered by calling a blocking recv. Another tests needed a Windows specific assert error message. Signed-off-by: Daniel Tacalau <[email protected]> * Added a note in the read/peek(_from) docs that on Windows if the data is larger than the buffer specified, the buffer is filled with the first part of the data, and peek returns the error WSAEMSGSIZE(10040). The excess data is lost. Make sure to always use a sufficiently large buffer to hold the maximum UDP packet size, which can be up to 64 kilobytes in size. Signed-off-by: Daniel Tacalau <[email protected]> * Added two new tests verifying ET behavior for UDP sockets events. These tests rely on having a blocking recv(_from) reregistering the socket and reseting the interests. Signed-off-by: Daniel Tacalau <[email protected]> * replaces 64 kilobytes with 65536 bytes in the notes Signed-off-by: Daniel Tacalau <[email protected]> * changes after review for new et tests: removed unneeded block, expect a writable before actual write Signed-off-by: Daniel Tacalau <[email protected]>

dtacalau marked this pull request as ready for review November 25, 2019 16:34

Thomasdezeeuw reviewed Nov 25, 2019

View reviewed changes

dtacalau force-pushed the issue_1080 branch 3 times, most recently from adbbb4b to ceb9bd5 Compare November 25, 2019 20:42

Thomasdezeeuw reviewed Nov 27, 2019

View reviewed changes

Thomasdezeeuw requested changes Nov 27, 2019

View reviewed changes

fixed assert arguments order

2972f2b

Signed-off-by: Daniel Tacalau <[email protected]>

dtacalau force-pushed the issue_1080 branch from ceb9bd5 to 4a10e18 Compare November 27, 2019 20:54

dtacalau changed the title ~~reregister on partial reads/write for UDP sockets~~ fixed issue #1080, re-enable failing udp tests on windows Nov 27, 2019

dtacalau requested a review from Thomasdezeeuw November 27, 2019 21:03

carllerche approved these changes Nov 27, 2019

View reviewed changes

dtacalau added 3 commits November 27, 2019 23:36

fix issue tokio-rs#1080, re-enabled and fixed UDP tests failing on Wi…

8f4884c

…ndows. One tests needed to have interests reregistered by calling a blocking recv. Another tests needed a Windows specific assert error message. Signed-off-by: Daniel Tacalau <[email protected]>

Added two new tests verifying ET behavior for UDP sockets events. The…

d21c006

…se tests rely on having a blocking recv(_from) reregistering the socket and reseting the interests. Signed-off-by: Daniel Tacalau <[email protected]>

dtacalau force-pushed the issue_1080 branch from 0c51644 to d21c006 Compare November 27, 2019 21:37

Thomasdezeeuw approved these changes Nov 28, 2019

View reviewed changes

Thomasdezeeuw mentioned this pull request Nov 28, 2019

Investigate and document UDP recv/peek(_from) behavior on *nix when using a smaller buffer. #1174

Closed

dtacalau added 2 commits November 28, 2019 17:42

replaces 64 kilobytes with 65536 bytes in the notes

e5256e1

Signed-off-by: Daniel Tacalau <[email protected]>

changes after review for new et tests: removed unneeded block, expect…

3fc91d5

… a writable before actual write Signed-off-by: Daniel Tacalau <[email protected]>

dtacalau requested a review from Thomasdezeeuw November 28, 2019 15:47

Thomasdezeeuw merged commit 8603167 into tokio-rs:master Nov 30, 2019

dtacalau mentioned this pull request Nov 30, 2019

Investigate UdpSocket test failures #1080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed issue #1080, re-enable failing udp tests on windows #1167

fixed issue #1080, re-enable failing udp tests on windows #1167

dtacalau commented Nov 25, 2019 •

edited

Loading

dtacalau commented Nov 25, 2019

Thomasdezeeuw Nov 25, 2019

dtacalau Nov 25, 2019

dtacalau Nov 27, 2019

Thomasdezeeuw Nov 27, 2019

Thomasdezeeuw Nov 27, 2019

dtacalau Nov 27, 2019

Thomasdezeeuw Nov 27, 2019

dtacalau Nov 27, 2019

dtacalau Nov 27, 2019

Thomasdezeeuw Nov 27, 2019

dtacalau Nov 27, 2019

carllerche Nov 27, 2019

Thomasdezeeuw Nov 28, 2019

dtacalau Nov 28, 2019

Thomasdezeeuw Nov 27, 2019

dtacalau commented Nov 27, 2019

dtacalau commented Nov 27, 2019

dtacalau commented Nov 27, 2019

carllerche left a comment

Thomasdezeeuw left a comment

Thomasdezeeuw Nov 28, 2019

Thomasdezeeuw Nov 28, 2019

dtacalau Nov 28, 2019

Thomasdezeeuw Nov 28, 2019

Thomasdezeeuw Nov 28, 2019

dtacalau Nov 28, 2019

Thomasdezeeuw Nov 28, 2019

dtacalau Nov 28, 2019

dtacalau commented Nov 28, 2019

fixed issue #1080, re-enable failing udp tests on windows #1167

fixed issue #1080, re-enable failing udp tests on windows #1167

Conversation

dtacalau commented Nov 25, 2019 • edited Loading

dtacalau commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtacalau commented Nov 27, 2019

dtacalau commented Nov 27, 2019

dtacalau commented Nov 27, 2019

carllerche left a comment

Choose a reason for hiding this comment

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtacalau commented Nov 28, 2019

dtacalau commented Nov 25, 2019 •

edited

Loading